Fix setting user overrides for dates on the user detail page for set versions. #2345

drgrice1 · 2024-02-27T14:17:00Z

This addresses issue #2340.

Currently if an instructor unchecks a checkbox for a user's versioned set on the user detail page (the page you can reach by clicking on the "Assigned Sets" column for a user in the Accounts Manager), then when dates are checked (in the checkDates method of UserDetail.pm), the dates from the global set record are used. If those are valid dates (they usually will be in this case), then those valid dates are returned with no error. Thus the database will be updated. However, the logic used to determine how the database is updated does not match the logic used to check the sets, and since the override checkbox is not checked, the date in the user's versioned set record is set to NULL. So for versioned sets this makes it so that if the checkbox is not checked or the override date is not set, then the global set date is NOT used for a fallback. Instead 0 is used for a fallback. Since that is not a valid date, this forces an error, and the database is not updated.

This is actually a more general issue with the logic used when the dates are checked and the logic used when the database is updated not being the same. So that is fixed.

Additionally when the dates in the database are updated, there are cases where the user interface does not correctly reflect the changes made. One such case is for a regular set if the checkbox for a date is checked but the override date left empty. Then the global set date is used (meaning the user set value for that date is set to NULL in the database), however the checkbox remains checked. So the corresponding parameters are updated so that the changes are reflected when the page rerenders after saving.

Note that in the case that the dates are not updated in the database due to an error in the given dates, the user interface does not update to reflect the current database state. I don't think the user interface should be updated in this case, as the instructor might want an opportunity to correct the error and not have their changes wiped out.

I also noticed that the "User overrides" header was being displayed over the "Open", "Closes", "Answer" labels in the case that a set is not assigned to a user (and so there are no override fields). So that is not shown in that case anymore.

I also removed the 10 year limit on dates. I am not sure why we impose this limit. There is no reason to do so. In fact, it is quite useful in many situations to have a due date that is more than 10 years in the future. By the way, I was curious if these dates were affected by the year 2038 problem. So after removing this restriction I set a due date to be in 2050. This seems to work with no issue. So it seems that 64bit integers are being used in this case which are not affected by the year 2038 problem. So we have 292 million years before this will be an issue again.

Edit: Additional changes added.

The header on the user override dates now read "User Overrides" unless it is a test version in which case it is "User's Test Version Dates".

The header on the dates for the global set reads "Test Dates" for tests and "Assignment Dates" for other assignments.

The placeholder in the user override date inputs now reads "Required" for test versions (except for the reduced scoring date which reads "Test Default"), "Test Default" for test template sets, and "Assignment Default" for other assignments. Furthermore, for test versions the dates have the required attribute (except for the reduced scoring date), so the form will not submit unless those dates are provided. Note that the reduced scoring date works differently than the other dates, and is not filled in when the test is submitted like the other dates are.

The date override check boxes have been removed.

Remove the "s" from the end of the "Close" date label. That was inconsistent with the other date labels.

The class dates for the set are now in readonly inputs instead of just being text. This is the same as on the set detail page. One advantage of this is that it makes it easier to cut and paste those dates.

There is also a small tweak to the datepicker.js (which is used on this page as well as on the sets manager page and the set detail page). When flatpickr adds the date picker, it hides the input originally on the page, and adds another input. This moves the id from the original input onto the added flatpickr input so that labels still work. The ids aren't used for anything else after the date picker javascript has found them. Also remove the placeholder that flatpickr copies to the new input since that isn't valid on a hidden input.

drgrice1 · 2024-02-27T14:22:39Z

lib/WeBWorK/ContentGenerator/Instructor/UserDetail.pm

 										} else {
 											$setVersionRecord->$field(undef);
+
+											# Update the parameters so the user interface reflects the changes made.
+											$c->param("set.$setID,v$ver.$field.override", undef);
+											$c->param("set.$setID,v$ver.$field",          undef);
 										}


Actually, with the change made to the checkDates method, this case is impossible now. Because if $c->param("set.$setID,v$ver.$field.override") is undefined or $c->param("set.$setID,v$ver.$field") is empty, then checkDates will return an error. So this code block won't even be entered. So I am going to delete this block. The dates for a set version should never be set to NULL in any case.

One other change was now added. If the instructor deletes a date for a set version and saves (in which case no changes will be made in the database), then the date in the database is put back into that date input when the page is re-renderred. So the inputs for a versioned set are never left empty.

Alex-Jordan

I tested against the original reported issue, and it blocked the save as you describe.

Since you are already adjusting the "User Overrides" header, what do you think about changing that term for when it is a test version? I'm interested in making it more clear to the uninitiated what's going on with tests and versions. In those rows, "Set values" could become something more like "Parent test values", and User Overrides" could become something like "Test version for user". Maybe those are not the best but surely something is better than what we have there now.

drgrice1 · 2024-02-29T22:32:35Z

Sounds good to rename them. How about "Version overrides" instead of "Test version for user" though?

drgrice1 · 2024-02-29T22:37:30Z

Or "User version values", or "Test version values"? I guess in a sense these are not exactly overrides. They are in terms of how the database works, but they aren't in the sense that it doesn't make sense for a test version to fill in the parent test values for these. In fact, would it be a good idea to even drop the "Parent test values" (or "Set values") part of the dates table for a test version?

drgrice1 · 2024-02-29T22:42:25Z

Another way to go that might clarify things in general would be to not use the terminology "Set values" anywhere. Instead use "Course values". Also, maybe "values" is wrong entirely. These are all "dates", so why not just say so. So use "Course dates" instead.

So regular sets and test template sets would have "User overrides" and "Course dates", and test versions would only show the user dates with the header "Test version dates".

How does that sound?

pstaabp

Looks good.

drgrice1 · 2024-02-29T22:51:00Z

So taking the further suggesting further, if we go that direction and remove the course dates from test versions, then the override check boxes would also be removed. With this pull request, they can't effectively be unchecked anyway. If they are unchecked the save of the dates is refused. So there is no point in them being there.

pstaabp · 2024-02-29T22:52:02Z

So regular sets and test template sets would have "User overrides" and "Course dates", and test versions would only show the user dates with the header "Test version dates".

This sounds clearer than the vague term "values".

drgrice1 · 2024-02-29T23:04:53Z

Here is a screenshot after making the changes I suggested above. This shows all of the possibilities. The first set is a regular homework set, the second is a test template set, the third a test version, and the fourth a set that is not assigned to this user. If this sounds good, then I will add it to this pull request.

Alex-Jordan · 2024-02-29T23:27:39Z

I think these are all good. More thoughts I have:

"Assignment dates" instead of "Course dates" on a homework set
"Test dates" instead of "Course dates" on a test template set
"User's test version dates" instead of "Test version dates"
Capitalize both words in the headers
aligning the date fields, even in the test version row to align with the other rows
Do we need checkmarks ever? Imagine there are no checkmarks. If the use saves and the field is empty, that could match the current behavior of unchecking. Or if the user saves and a field is nonempty, that should imply the item is checked.

I don't feel strongly about any of these if you don't like them.

drgrice1 · 2024-02-29T23:48:56Z

* "Assignment dates" instead of "Course dates" on a homework set
* "Test dates" instead of "Course dates" on a test template set
* "User's test version dates" instead of "Test version dates"

I am fine with these. I think "User's Test Version Dates" is a bit long, but it seems to fit okay, I guess.

* Capitalize both words in the headers

I agree on this.

* aligning the date fields, even in the test version row to align with the other rows

I realized that I went to far in my screenshot, and removed the date labels ("Open", "Close", etc.). That will obviously be added back, and the helps to line this up a little more. With those added back, if the check boxes are also removed in general, then this will just line up without further effort.

* Do we need checkmarks _ever_? Imagine there are no checkmarks. If the use saves and the field is empty, that could match the current behavior of unchecking. Or if the user saves and a field is nonempty, that should imply the item is checked.

I am okay with this, but it does seem that it makes it less clear how to remove a date override to make this change. Perhaps I am wrong on this? Note, that this request will take a rather heavy rewrite of the module in general though.

Alex-Jordan · 2024-03-01T00:18:46Z

Note, that this request will take a rather heavy rewrite of the module in general though.

Let's forget about it for now. If that change is made, it should be consistent everywhere in other places that have these checkboxes.

I've never understood why they are needed though. Fields are on the screen. A user can change what is in those fields. With some systems, that is all that is needed and there is not even a need for a save button. But forget about that, we will keep the save button. So then you save, and whatever is in the fields at that time is saved (if it validates).

drgrice1 · 2024-03-01T00:37:52Z

I've never understood why they are needed though. Fields are on the screen. A user can change what is in those fields. With some systems, that is all that is needed and there is not even a need for a save button. But forget about that, we will keep the save button. So then you save, and whatever is in the fields at that time is saved (if it validates).

This page is designed to be like the dates on the "Set Manager" page. On that page there are cases where the check boxes are necessary. Namely when the override field is a drop down menu. Although, now that I write that I realize that you could add a "use set default" option to the select in that case.

For this page, it would not be that hard to remove the check boxes. Also, the only way to get nice alignment of dates for versioned sets with out check boxes with the other sets is to also not have check boxes for the other sets.

By the way, not having a save button is not a good idea in my opinion. The systems that do that are sending a network request each time the input changes to save the data to the server. That results in excessive network traffic. A more important reason though is just the ease of reverting if you decide you don't like the changes. For a page like this that is useful. You can just reload the page, and get the current settings in the database back. Most systems that I have seen similar to this have a save button for that reason (Canvas, Moodle, Pearson MyLab, and Web Assign all have save buttons for pages where you mass edit assignment dates or settings).

Alex-Jordan · 2024-03-01T00:55:55Z

This page is designed to be like the dates on the "Set Manager" page.

Do you mean the Set Details page for a user (or subset of users)? That's what I was thinking when I mentioned a change like that should happen in other places. Those dropdowns could have "None Specified" as an option, which is the placeholder text in the neighboring text fields.

If there are location restrictions, then "Restrict Locations" is a multi-select that appears there. I can't seem to unselect something there so either things would need to be made "unselectable" there or it would need "use set default"/"None Specified" as well.

I agree about save buttons. I feel unsure of myself when they are not there.

drgrice1 · 2024-03-01T01:16:13Z

It seems to me that the placeholders for the text inputs on the "Set Detail" page when editing for a user would be more clear if they read "Set Defaults" (or "Assignment Defaults"). or something that indicates what it means if the input is left blank. Particularly if the check boxes are removed.

drgrice1 · 2024-03-01T02:36:16Z

I implemented the changes we discussed. Removing the check boxes wasn't as big of a rewrite as I though, so I did it. Make it a TODO to do the same (in another pull request) for the set detail page.

Alex-Jordan · 2024-03-01T03:36:20Z

if they read "Set Defaults" (or "Assignment Defaults").

That sounds good.

…

On Thu, Feb 29, 2024, 5:16 PM Glenn Rice ***@***.***> wrote: It seems to me that the placeholders for the text inputs on the "Set Detail" page when editing for a user would be more clear if they read "Set Defaults" (or "Assignment Defaults"). or something that indicates what it means if the input is left blank. Particularly if the check boxes are removed. — Reply to this email directly, view it on GitHub <https://protect2.fireeye.com/v1/url?k=31323334-501d2dca-3132feb7-454455534531-10d2afc0d7d97e57&q=1&e=4c08192a-6e50-4955-8ac4-644914dece12&u=https%3A%2F%2Fgithub.com%2Fopenwebwork%2Fwebwork2%2Fpull%2F2345%23issuecomment-1972268074>, or unsubscribe <https://protect2.fireeye.com/v1/url?k=31323334-501d2dca-3132feb7-454455534531-d29626799568bb46&q=1&e=4c08192a-6e50-4955-8ac4-644914dece12&u=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FABEDOAFYFGOEZQCDKJ6NHC3YV7JGRAVCNFSM6AAAAABD4GHPGSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNZSGI3DQMBXGQ> . You are receiving this because you commented.Message ID: ***@***.***>

Alex-Jordan · 2024-03-01T05:44:07Z

This looks good, but I started playing with things and it led to a PR I just opened to your branch.

drgrice1 · 2024-03-01T12:51:45Z

There is a downside of removing the check boxes. If an instructor wants to remove all override dates for a student, then with the check boxes all that needs to be done is to uncheck all check boxes. Without the check boxes the values in all of the inputs have to be deleted which does take more work. Admittedly removing override dates is something that generally shouldn't be done. If you have extended the due date on an assignment, then removing that extension would be bad practice.

drgrice1 · 2024-03-01T18:04:51Z

There is bad news for removing check boxes on the problem set detail page when editing a set for a user. I am not saying that it can not be done, but it will be quite challenging and will take quite a bit of work. The most challenging part will be dealing with test versions. To make it all work it really will take a rather hefty rewrite of that module, and much care will be needed to get it right.

Alex-Jordan · 2024-03-01T18:25:31Z

OK, I regret making suggestions that take this so far astray from the bugfix. What do you think is best to tie a bow on this PR? Revert the checkboxes and punt the other things in that PR I opened?

drgrice1 · 2024-03-01T18:29:16Z

I think it is fine to continue on the path of this pull request and what we have done. This page does not have to be entirely consistent with the set detail page. Eventually, it would be nice to get rid of the check boxes there too. It will clean up the page quite a bit. Everything is so tight because so much is squeezed into that page, and getting rid of an unnecessary UI element is helpful for that.

drgrice1 · 2024-03-01T21:59:19Z

I added translation of the check date error messages and a few other tweaks.

Alex-Jordan · 2024-03-02T00:18:21Z

I checked it out and tested, and it's all good. Consider my approval renewed!

I'll let @pstaabp (or anyone) take a look again before merging though.

…versions. This addresses issue openwebwork#2340. Currently if an instructor unchecks a checkbox for a user's versioned set on the user detail page (the page you can reach by clicking on the "Assigned Sets" column for a user in the Accounts Manager), then when dates are checked (in the `checkDates` method of `UserDetail.pm`), the dates from the global set record are used. If those are valid dates (they usually will be in this case), then those valid dates are returned with no error. Thus the database will be updated. However, the logic used to determine how the database is updated does not match the logic used to check the sets, and since the override checkbox is not checked, the date in the user's versioned set record is set to NULL. So for versioned sets this makes it so that if the checkbox is not checked or the override date is not set, then the global set date is NOT used for a fallback. Instead 0 is used for a fallback. Since that is not a valid date, this forces an error, and the database is not updated. This is actually a more general issue with the logic used when the dates are checked and the logic used when the database is updated not being the same. So that is fixed. Additionally when the dates in the database are updated, there are cases where the user interface does not correctly reflect the changes made. One such case is for a regular set if the checkbox for a date is checked but the override date left empty. Then the global set date is used (meaning the user set value for that date is set to NULL in the database), however the checkbox remains checked. So the corresponding parameters are updated so that the changes are reflected when the page rerenders after saving. Note that in the case that the dates are not updated in the database due to an error in the given dates, the user interface does not update to reflect the current database state. I don't think the user interface should be updated in this case, as the instructor might want an opportunity to correct the error and not have their changes wiped out. I also noticed that the "User overrides" header was being displayed over the "Open", "Closes", "Answer" labels in the case that a set is not assigned to a user (and so there are no override fields). So that is not shown in that case anymore. I also removed the 10 year limit on dates. I am not sure why we impose this limit. There is no reason to do so. In fact, it is quite useful in many situations to have a due date that is more than 10 years in the future. By the way, I was curious if these dates were affected by the year 2038 problem. So after removing this restriction I set a due date to be in 2050. This seems to work with no issue. So it seems that 64bit integers are being used in this case which are not affected by the year 2038 problem. So we have 292 million years before this will be an issue again.

The header on the user override dates now read "User Overrides" unless it is a test version in which case it is "User's Test Version Dates". The header on the dates for the global set reads "Test Dates" for tests and "Assignment Dates" for other assignments. The placeholder in the user override date inputs now reads "Test Default" for tests, and "Assignment Default" for other assignments. The date override checkboxes have been removed. Remove the "s" from the end of the "Close" date label. That was inconsistent with the other date labels. The class dates for the set are now in readonly inputs instead of just being text. This is the same as on the set detail page. One advantage of this is that it makes it easier to cut and paste those dates. There is also a small tweak to the datepicker.js (which is used on this page as well as on the sets manager page and the set detail page). When flatpickr adds the date picker, it hides the input originally on the page, and adds another input. This moves the id from the original input onto the added flatpickr input so that labels still work. The ids aren't used for anything else after the date picker javascript has found them. Also remove the placeholder that flatpickr copies to the new input since that isn't valid on a hidden input.

…dvice. The advice is to consider alternatve measures to changing dates for test versions. Also remove the `ms-auto` class added to the dates table for test versions. The alignment is still not right with that.

pstaabp · 2024-03-02T12:54:15Z

All of these are great additions. The help box clarifies the issued that this started from and I think the label of dates are much clearer now.

It still references the check boxes for the date overrides which no longer exist. Also translate the error messages from when the dates are checked.

drgrice1 · 2024-03-03T01:50:20Z

I made one last minor change. I didn't like that if the open, close, or answer date is not set it says Set [_1] has errors in its dates: open_date |[_2]|, due date |[_3]|, answer_date |[_4]|, where the dates shown are the unix timestamps. So now those are formatted dates if set, and "required" if not. Also, I changed "due date" to "close date" in the messages, since that is what this date is labeled in the user interface.

Alex-Jordan · 2024-03-05T21:08:59Z

Since @pstaabp gave this another look, I'm going to proceed with merging it.

drgrice1 commented Feb 27, 2024

View reviewed changes

drgrice1 force-pushed the bugfix/date-overrides-versioned-user-set branch 5 times, most recently from 4064931 to 10b432c Compare February 29, 2024 21:59

Alex-Jordan approved these changes Feb 29, 2024

View reviewed changes

pstaabp approved these changes Feb 29, 2024

View reviewed changes

drgrice1 force-pushed the bugfix/date-overrides-versioned-user-set branch from 10b432c to 38c974c Compare March 1, 2024 02:33

drgrice1 force-pushed the bugfix/date-overrides-versioned-user-set branch 2 times, most recently from 567928d to bf9cda3 Compare March 1, 2024 03:02

drgrice1 force-pushed the bugfix/date-overrides-versioned-user-set branch 3 times, most recently from eadaf88 to f84d235 Compare March 1, 2024 03:57

drgrice1 and others added 4 commits March 1, 2024 18:32

rearrange tables in user sets page

4164a7c

Add a popover to the user detail date tables for test versions with a…

f70b0c9

…dvice. The advice is to consider alternatve measures to changing dates for test versions. Also remove the `ms-auto` class added to the dates table for test versions. The alignment is still not right with that.

drgrice1 force-pushed the bugfix/date-overrides-versioned-user-set branch from 2832346 to d566c07 Compare March 2, 2024 00:33

drgrice1 force-pushed the bugfix/date-overrides-versioned-user-set branch from d566c07 to 2063ffa Compare March 3, 2024 01:46

Fix the help for the User Detail page.

5200f0c

It still references the check boxes for the date overrides which no longer exist. Also translate the error messages from when the dates are checked.

drgrice1 force-pushed the bugfix/date-overrides-versioned-user-set branch from 2063ffa to 5200f0c Compare March 3, 2024 01:49

drgrice1 mentioned this pull request Mar 3, 2024

Remove user override check boxes when editing set details for a user. #2348

Merged

Alex-Jordan merged commit b8e51f1 into openwebwork:develop Mar 5, 2024
2 checks passed

drgrice1 deleted the bugfix/date-overrides-versioned-user-set branch March 5, 2024 21:12

pstaabp mentioned this pull request Mar 11, 2024

instructors can nullify test version dates #2340

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix setting user overrides for dates on the user detail page for set versions. #2345

Fix setting user overrides for dates on the user detail page for set versions. #2345

drgrice1 commented Feb 27, 2024 •

edited

Loading

drgrice1 Feb 27, 2024

drgrice1 Feb 27, 2024

Alex-Jordan left a comment

drgrice1 commented Feb 29, 2024

drgrice1 commented Feb 29, 2024

drgrice1 commented Feb 29, 2024

pstaabp left a comment

drgrice1 commented Feb 29, 2024

pstaabp commented Feb 29, 2024

drgrice1 commented Feb 29, 2024

Alex-Jordan commented Feb 29, 2024

drgrice1 commented Feb 29, 2024

Alex-Jordan commented Mar 1, 2024

drgrice1 commented Mar 1, 2024

Alex-Jordan commented Mar 1, 2024

drgrice1 commented Mar 1, 2024

drgrice1 commented Mar 1, 2024

Alex-Jordan commented Mar 1, 2024 via email

Alex-Jordan commented Mar 1, 2024

drgrice1 commented Mar 1, 2024

drgrice1 commented Mar 1, 2024

Alex-Jordan commented Mar 1, 2024

drgrice1 commented Mar 1, 2024

drgrice1 commented Mar 1, 2024

Alex-Jordan commented Mar 2, 2024

pstaabp commented Mar 2, 2024

drgrice1 commented Mar 3, 2024

Alex-Jordan commented Mar 5, 2024

Fix setting user overrides for dates on the user detail page for set versions. #2345

Fix setting user overrides for dates on the user detail page for set versions. #2345

Conversation

drgrice1 commented Feb 27, 2024 • edited Loading

drgrice1 Feb 27, 2024

Choose a reason for hiding this comment

drgrice1 Feb 27, 2024

Choose a reason for hiding this comment

Alex-Jordan left a comment

Choose a reason for hiding this comment

drgrice1 commented Feb 29, 2024

drgrice1 commented Feb 29, 2024

drgrice1 commented Feb 29, 2024

pstaabp left a comment

Choose a reason for hiding this comment

drgrice1 commented Feb 29, 2024

pstaabp commented Feb 29, 2024

drgrice1 commented Feb 29, 2024

Alex-Jordan commented Feb 29, 2024

drgrice1 commented Feb 29, 2024

Alex-Jordan commented Mar 1, 2024

drgrice1 commented Mar 1, 2024

Alex-Jordan commented Mar 1, 2024

drgrice1 commented Mar 1, 2024

drgrice1 commented Mar 1, 2024

Alex-Jordan commented Mar 1, 2024 via email

Alex-Jordan commented Mar 1, 2024

drgrice1 commented Mar 1, 2024

drgrice1 commented Mar 1, 2024

Alex-Jordan commented Mar 1, 2024

drgrice1 commented Mar 1, 2024

drgrice1 commented Mar 1, 2024

Alex-Jordan commented Mar 2, 2024

pstaabp commented Mar 2, 2024

drgrice1 commented Mar 3, 2024

Alex-Jordan commented Mar 5, 2024

drgrice1 commented Feb 27, 2024 •

edited

Loading